AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed

What was reviewed

Skipped (out of scope): pnpm-lock.yaml, e2e/RichText.spec.js-snapshots/viewCodeDialog-chromium-linux.png

Findings

🔶 Medium — registerCustomFormats mutates global Quill registry per instance

File: src/utils/customPluginRegisters.ts lines 33–70
Problem: Quill.register() is a global singleton mutation. registerCustomFormats is called inside each Editor's useLayoutEffect (see Editor.tsx line ~136). If two RichText widgets exist on the same page — one in inline mode and one in class mode — whichever mounts last overwrites the global attributors for all instances. The first widget will silently switch to the wrong format mode without re-rendering.
Fix: Gate registration so it only runs once per format value (e.g. track which format was last registered in a module-level variable and skip if already registered), or document clearly that mixing inline and class mode widgets on the same page is unsupported and add a runtime warning:

let registeredFormat: "inline" | "class" | null = null;

export function registerCustomFormats(props: MxQuillModulesOptions): void {
    if (registeredFormat === props.styleDataFormat) return;
    registeredFormat = props.styleDataFormat;
    // ... existing registration logic
}

🔶 Medium — Missing E2E snapshot baselines for class-mode tests

File: e2e/RichText.spec.js lines 46, 67
Problem: Two new toHaveScreenshot() calls reference classModeEditor.png and classModeViewCodeDialog.png, but neither file exists in e2e/RichText.spec.js-snapshots/. Playwright requires a committed baseline PNG. These tests will fail on the first CI run with "missing snapshot" rather than generating a useful baseline.
Fix: Run pnpm e2edev -- --update-snapshots against the test project to generate the baselines, then commit the resulting PNG files alongside this PR.

🔶 Medium — serif font missing from SCSS $fonts map

File: src/ui/variables.scss lines 14–27
Problem: FONT_LIST in fonts.ts contains 13 fonts including { value: "serif", ... }. The $fonts SCSS map has only 12 entries — serif is absent. FontClassAttributor.add() appends a font-family-serif class to the DOM node, but RichTextFormatStyle.scss generates no .font-family-serif rule, so the font silently fails to apply in class mode.
Fix: Add serif to the $fonts map:

$fonts: (
    // ... existing entries
    serif: serif,
    trebuchet: $font-trebuchet
);

⚠️ Low — Orphaned $color variable in RichTextFormatStyle.scss

File: src/ui/RichTextFormatStyle.scss line 3
Note: $color: #3d1466; is declared but never referenced in the file. #3d1466 already appears as the last entry in $colors in variables.scss. Safe to remove to avoid confusion.

⚠️ Low — .code-editor-container missing position: relative

File: src/components/ModalDialog/Dialog.scss lines 28–46
Note: .code-input and .code-preview are position: absolute, which requires a positioned ancestor to stack correctly. .code-editor-container has no position: relative. Their stacking will fall through to whatever ancestor happens to be positioned, likely the modal overlay. This may work coincidentally with the current markup but is fragile. Add position: relative to .code-editor-container.

⚠️ Low — Buffer textarea not hidden from assistive technology

File: src/components/ModalDialog/ViewCodeDialog.tsx line 729
Note: The disabled buffer <textarea> is an invisible overlay used purely for scroll-sync height measurement. It will still be read by screen readers as a disabled form field. Add aria-hidden="true" to exclude it from the accessibility tree.

Positives

• Removing the CodeMirror dependency (and its four transitive packages) meaningfully reduces bundle size for a CSP-sensitive use case.
• The normalizeStyleAndClassAttribute utility is bidirectional — it handles both inline→class and class→inline conversion in a single function, which is clean and testable.
• The three new unit test files use JSDOM directly without needing a Quill instance, making them fast and reliable. Good coverage of edge cases (RTL padding-right, zero-indent skip, custom fonts).
• Moving side-effectful Quill.register() calls out of fontsize.ts import time is the right direction — modules that register globals on import are hard to test and control.
• The useEffect cleanup in ViewCodeDialog.tsx correctly captures codeRef.current before the async gap and uses delete codeElement.dataset.highlighted to let highlight.js re-highlight on the next render.

AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed

What was reviewed

Skipped (out of scope): pnpm-lock.yaml, e2e/RichText.spec.js-snapshots/viewCodeDialog-chromium-linux.png

Findings

🔶 Medium — Missing screenshot baselines for two new E2E toHaveScreenshot tests

File: e2e/RichText.spec.js lines 46 and 67
Problem: Two new tests call toHaveScreenshot('classModeEditor.png') and toHaveScreenshot('classModeViewCodeDialog.png'), but neither baseline PNG exists in e2e/RichText.spec.js-snapshots/. Playwright fails with "missing snapshot" when no baseline is present. These tests will block CI on first run.
Fix: Run pnpm e2edev against the test project to generate the baselines, then commit the two PNG files:

e2e/RichText.spec.js-snapshots/classModeEditor-chromium-linux.png
e2e/RichText.spec.js-snapshots/classModeViewCodeDialog-chromium-linux.png

🔶 Medium — Quill.register is a global singleton; two widgets with different styleDataFormat will conflict

File: src/utils/customPluginRegisters.ts lines 50–80; src/components/Editor.tsx line 140
Problem: registerCustomFormats calls Quill.register(...) on the global Quill registry inside each editor's useLayoutEffect. If a page contains two RichText widgets — one in inline mode and one in class mode — the second widget to mount overwrites the format registrations for both. The last-registered attributor wins globally (e.g. FontClassAttributor overwrites FontStyleAttributor even for the inline widget).
Fix: Guard against re-registration using a module-level flag, or check the current registered format before calling Quill.register again. Example:

let registeredFormat: "inline" | "class" | null = null;

export function registerCustomFormats(props: MxQuillModulesOptions): void {
    if (registeredFormat === props.styleDataFormat) return;
    registeredFormat = props.styleDataFormat;
    // ... existing logic
}

Note: even with this guard the first-registered mode wins for the whole page, which may still be wrong if two widgets have different modes. The underlying issue is that Quill's global registry does not support per-instance format variants. This is worth documenting as a known limitation in the PR description.

⚠️ Low — Unused top-level $color variable shadows the @each loop variable

File: src/ui/RichTextFormatStyle.scss line 3
Note: $color: #3d1466; at the top of the file is never used — the last entry in the $colors list in variables.scss is already #3d1466. It also shadows the $color iteration variable in the @each $color in $colors loop on line 21 (SCSS loop variables are locally scoped, so this is not a bug, but it is confusing). Remove the unused variable.

⚠️ Low — Buffer textarea for scroll sync lacks aria-hidden

File: src/components/ModalDialog/ViewCodeDialog.tsx lines 64–70
Note: The disabled buffer textarea (used purely for scroll-sync measurement) is hidden via opacity: 0 in CSS, but it has no aria-hidden="true". Screen readers may still traverse to it and announce the duplicated content. Add aria-hidden="true" to the element.

<textarea
    aria-hidden="true"
    spellCheck={false}
    value={formState.src}
    id="rich-text-code-input-buffer"
    disabled
    className="code-editor code-buffer"
/>

⚠️ Low — Uppercase HTML tag selectors in SCSS

File: src/ui/RichText.scss lines 175–176
Note: VIDEO, IFRAME { pointer-events: none; } uses uppercase selectors. While valid CSS, all other selectors in the codebase use lowercase. Change to video, iframe for consistency with the rest of the stylesheet.

⚠️ Low — test.describe.configure({ mode: "serial" }) serialises the entire outer describe block

File: e2e/RichText.spec.js line 32
Note: Placing .configure({ mode: "serial" }) at the outer test.describe("RichText", ...) level serialises all tests in that block, including the pre-existing parallel tests. If the intent was only to serialise the new class-mode tests, extract them into a nested test.describe("class mode", ...) block and apply the config there. Otherwise this will slow down CI for all RichText E2E runs.

Positives

• Excellent unit test coverage for the new utility classes (FontClassAttributor, normalizeStyleAndClassAttribute, CustomListItem.formats) — tests cover edge cases like RTL padding, zero-indent, custom fonts, and unknown font values.
• Replacing CodeMirror (a heavy dependency with CSP-incompatible new Function evaluation) with a plain <textarea> + highlight.js is the right call for CSP compliance and bundle size.
• normalizeStyleAndClassAttribute handles bidirectional conversion (class→inline and inline→class) so pasted content is normalised regardless of the source editor mode.
• Moving side-effectful module-level Quill.register calls out of fontsize.ts and into registerCustomFormats is a clean improvement — module imports no longer have hidden global side effects.
• CHANGELOG.md entry is present and correctly covers all three change categories (Added, Fixed, Changed).